interpret: adjust vtable validity check for higher-ranked types #135296

lukas-code · 2025-01-09T17:53:12Z

What

Transmuting between trait objects where a generic argument or associated type only differs in bound regions (not bound at or above the trait object's binder) is now UB. For example

transmuting between &dyn Trait<for<'a> fn(&'a u8)> and &dyn Trait<fn(&'static u8)> is UB.
transmuting between &dyn Trait<Assoc = for<'a> fn(&'a u8)> and &dyn Trait<Assoc = fn(&'static u8)> is UB.
transmuting between &dyn Trait<for<'a> fn(&'a u8) -> (&'a u8, &'static u8)> and &dyn Trait<for<'a> fn(&'a u8) -> (&'static u8, &'a u8)> is UB.

Transmuting between subtypes (in either direction) is still allowed, which means that bound regions that are bound at or above the trait object's binder can still be changed:

transmuting between &dyn for<'a> Trait<fn(&'a u8)> and &dyn for Trait<fn(&'static u8)> is fine.
transmuting between &dyn for<'a> Trait<dyn Trait<fn(&'a u8)>> and &dyn for Trait<dyn Trait<fn(&'static u8)>> is fine.

Why

Very similar to #120217 and #120222, changing a trait object's generic argument to a type that only differs in bound regions can still affect the vtable layout and lead to segfaults at runtime (for an example see src/tools/miri/tests/fail/validity/dyn-trait-leak-check.rs).

Since we already already require that the trait object predicates must be equal modulo bound regions, it is only natural to extend this check to also require type equality considering bound regions.

However, it also makes sense to allow transmutes between a type and a subtype thereof. For example &dyn for<'a> Trait<&'a u8> is a subtype of &dyn Trait<&'static ()> and they are guaranteed to have the same vtable, so it makes sense to allow this transmute. So that's why bound lifetimes that are bound to the trait object itself are treated as free lifetime for the purpose of this check.

Note that codegen already relies on the property that subtyping cannot change the the vtable and this is asserted here (note the leak check):

rust/compiler/rustc_codegen_ssa/src/base.rs

Lines 106 to 153 in 251206c

    
           /// Codegen takes advantage of the additional assumption, where if the 
        
           /// principal trait def id of what's being casted doesn't change, 
        
           /// then we don't need to adjust the vtable at all. This 
        
           /// corresponds to the fact that `dyn Tr<A>: Unsize<dyn Tr<B>>` 
        
           /// requires that `A = B`; we don't allow *upcasting* objects 
        
           /// between the same trait with different args. If we, for 
        
           /// some reason, were to relax the `Unsize` trait, it could become 
        
           /// unsound, so let's validate here that the trait refs are subtypes. 
        
           pub fn validate_trivial_unsize<'tcx>( 
        
               tcx: TyCtxt<'tcx>, 
        
               source_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>, 
        
               target_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>, 
        
           ) -> bool { 
        
               match (source_data.principal(), target_data.principal()) { 
        
                   (Some(hr_source_principal), Some(hr_target_principal)) => { 
        
                       let (infcx, param_env) = 
        
                           tcx.infer_ctxt().build_with_typing_env(ty::TypingEnv::fully_monomorphized()); 
        
                       let universe = infcx.universe(); 
        
                       let ocx = ObligationCtxt::new(&infcx); 
        
                       infcx.enter_forall(hr_target_principal, |target_principal| { 
        
                           let source_principal = infcx.instantiate_binder_with_fresh_vars( 
        
                               DUMMY_SP, 
        
                               BoundRegionConversionTime::HigherRankedType, 
        
                               hr_source_principal, 
        
                           ); 
        
                           let Ok(()) = ocx.eq_trace( 
        
                               &ObligationCause::dummy(), 
        
                               param_env, 
        
                               ToTrace::to_trace( 
        
                                   &ObligationCause::dummy(), 
        
                                   hr_target_principal, 
        
                                   hr_source_principal, 
        
                               ), 
        
                               target_principal, 
        
                               source_principal, 
        
                           ) else { 
        
                               return false; 
        
                           }; 
        
                           if !ocx.select_all_or_error().is_empty() { 
        
                               return false; 
        
                           } 
        
                           infcx.leak_check(universe, None).is_ok() 
        
                       }) 
        
                   } 
        
                   (_, None) => true, 
        
                   _ => false, 
        
               } 
        
           }

Furthermore, we allow some pointer-to-pointer casts like *const dyn for<'a> Trait<&'a u8> to *const Wrapper<dyn Trait<&'static u8>> that instantiate the trait object binder and are currently lowered to a single pointer-to-pointer cast in MIR (CastKind::PtrToPtr) and not an unsizing coercion (CastKind::PointerCoercion(Unsize)), so the current MIR lowering of these would be UB if we didn't allow subtyping transmutes.

fixes #135230
cc @rust-lang/opsem
r? @compiler-errors for the implementation

rustbot · 2025-01-09T17:53:23Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

lcnr · 2025-01-10T10:35:25Z

compiler/rustc_const_eval/src/interpret/eval_context.rs

    #[instrument(level = "trace", skip(self), ret)]
-    pub(super) fn eq_in_param_env<T>(&self, a: T, b: T) -> bool
+    pub(super) fn relate_dyn_predicates_bivariantly<T>(


bivariance is different from being bidirectional subtypes

lcnr/random-rust-snippets#3

change it to eq_modulo_regions? 🤔

👍 on manually adding the leak-check here. The alternative would be to instantiate all regions with existentials and do a proper region check at the end, not totally sure whether a leak-check will always be sufficient given that it doesn't have to be catch all errors

bivariance is different from being bidirectional subtypes

Hm, this comment here made me believe that they are supposed to be the same:

rust/compiler/rustc_infer/src/infer/at.rs

Lines 241 to 246 in 336209e

// We could make this make sense but it's not readily

// exposed and I don't feel like dealing with it. Note

// that bivariance in general does a bit more than just

// *nothing*, it checks that the types are the same

// "modulo variance" basically.

ty::Bivariant => panic!("Bivariant given to `relate()`"),

I see now that bivariance as implemented does indeed just do nothing, so 👍 on renaming this to avoid confusion.

Also, after thinking about this more, I think we can also just structurally compare the predicates (after instantiating the binder + normalizing), no leak check required, because the soundness of TypeId requires structural equality anyway (#97156).

Is there any reason why this would be a bad idea?

Hm, this comment here made me believe that they are supposed to be the same:

This comment is wrong then 😁

Also, after thinking about this more, I think we can also just structurally compare the predicates (after instantiating the binder + normalizing), no leak check required, because the soundness of TypeId requires structural equality anyway (#97156).

Is there any reason why this would be a bad idea?

don't think so, would have some false negatives in generic code, but 🤷

Alright, replaced this with just == now.

RalfJung · 2025-01-11T16:46:43Z

compiler/rustc_const_eval/src/interpret/eval_context.rs

    #[instrument(level = "trace", skip(self), ret)]
-    pub(super) fn eq_in_param_env<T>(&self, a: T, b: T) -> bool
+    pub(super) fn relate_dyn_predicates_bivariantly<T>(


Is there a better place for this function? Somewhere in rustc_ty_utils or whatever? I am entirely clueless about its implementation so maybe const_eval is the wrong crate for this. ;)

This function is gone now, because turns out that this entire complicated operation is (almost) just equivalent to == ^^

src/tools/miri/tests/fail/validity/dyn-trait-leak-check.rs

src/tools/miri/tests/pass/validity/dyn-trait-bivariant-transmutes.rs

… bound vars

rustbot · 2025-01-11T21:25:54Z

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

lukas-code · 2025-01-11T21:27:31Z

(this now includes #135318 to avoid conflicts, sorry for the pings)

rustbot assigned compiler-errors Jan 9, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 9, 2025

lukas-code force-pushed the dyn-leak-check branch from dcdc06e to 0536ee7 Compare January 9, 2025 21:14

This comment has been minimized.

Sign in to view

lukas-code force-pushed the dyn-leak-check branch from 0536ee7 to a7d6eae Compare January 10, 2025 01:23

lcnr reviewed Jan 10, 2025

View reviewed changes

RalfJung reviewed Jan 11, 2025

View reviewed changes

compiler-errors added 6 commits January 11, 2025 18:53

Rework rustc_dump_vtable

d341178

Remove print_vtable_sizes

f786515

Do not treat vtable supertraits as distinct when bound with different…

571eba5

… bound vars

Normalize vtable entries before walking and deduplicating them

1583ee2

Use ExistentialTraitRef throughout codegen

76eb4f3

More assertions, tests, and miri coverage

1543bb4

lukas-code force-pushed the dyn-leak-check branch from a7d6eae to a7c9e79 Compare January 11, 2025 21:25

rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

lukas-code removed the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

lukas-code force-pushed the dyn-leak-check branch from a7c9e79 to cc20826 Compare January 11, 2025 21:36

rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

lukas-code removed the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

interpret: adjust vtable validity check for higher-ranked types

ed1eb06

lukas-code force-pushed the dyn-leak-check branch from cc20826 to ed1eb06 Compare January 11, 2025 21:42

rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

lukas-code removed the A-rustc-dev-guide Area: rustc-dev-guide label Jan 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret: adjust vtable validity check for higher-ranked types #135296

interpret: adjust vtable validity check for higher-ranked types #135296

lukas-code commented Jan 9, 2025 •

edited

Loading

rustbot commented Jan 9, 2025

This comment has been minimized.

lcnr Jan 10, 2025

lukas-code Jan 10, 2025

lcnr Jan 11, 2025

lukas-code Jan 11, 2025

RalfJung Jan 11, 2025

lukas-code Jan 11, 2025

rustbot commented Jan 11, 2025

lukas-code commented Jan 11, 2025

	/// Codegen takes advantage of the additional assumption, where if the
	/// principal trait def id of what's being casted doesn't change,
	/// then we don't need to adjust the vtable at all. This
	/// corresponds to the fact that `dyn Tr<A>: Unsize<dyn Tr<B>>`
	/// requires that `A = B`; we don't allow upcasting objects
	/// between the same trait with different args. If we, for
	/// some reason, were to relax the `Unsize` trait, it could become
	/// unsound, so let's validate here that the trait refs are subtypes.
	pub fn validate_trivial_unsize<'tcx>(
	tcx: TyCtxt<'tcx>,
	source_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
	target_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
	) -> bool {
	match (source_data.principal(), target_data.principal()) {
	(Some(hr_source_principal), Some(hr_target_principal)) => {
	let (infcx, param_env) =
	tcx.infer_ctxt().build_with_typing_env(ty::TypingEnv::fully_monomorphized());
	let universe = infcx.universe();
	let ocx = ObligationCtxt::new(&infcx);
	infcx.enter_forall(hr_target_principal, \|target_principal\| {
	let source_principal = infcx.instantiate_binder_with_fresh_vars(
	DUMMY_SP,
	BoundRegionConversionTime::HigherRankedType,
	hr_source_principal,
	);
	let Ok(()) = ocx.eq_trace(
	&ObligationCause::dummy(),
	param_env,
	ToTrace::to_trace(
	&ObligationCause::dummy(),
	hr_target_principal,
	hr_source_principal,
	),
	target_principal,
	source_principal,
	) else {
	return false;
	};
	if !ocx.select_all_or_error().is_empty() {
	return false;
	}
	infcx.leak_check(universe, None).is_ok()
	})
	}
	(_, None) => true,
	_ => false,
	}
	}

	// We could make this make sense but it's not readily
	// exposed and I don't feel like dealing with it. Note
	// that bivariance in general does a bit more than just
	// nothing, it checks that the types are the same
	// "modulo variance" basically.
	ty::Bivariant => panic!("Bivariant given to `relate()`"),

interpret: adjust vtable validity check for higher-ranked types #135296

Are you sure you want to change the base?

interpret: adjust vtable validity check for higher-ranked types #135296

Conversation

lukas-code commented Jan 9, 2025 • edited Loading

What

Why

rustbot commented Jan 9, 2025

This comment has been minimized.

lcnr Jan 10, 2025

Choose a reason for hiding this comment

lukas-code Jan 10, 2025

Choose a reason for hiding this comment

lcnr Jan 11, 2025

Choose a reason for hiding this comment

lukas-code Jan 11, 2025

Choose a reason for hiding this comment

RalfJung Jan 11, 2025

Choose a reason for hiding this comment

lukas-code Jan 11, 2025

Choose a reason for hiding this comment

rustbot commented Jan 11, 2025

lukas-code commented Jan 11, 2025

lukas-code commented Jan 9, 2025 •

edited

Loading